Walkthrough

Adds schema-aware hooks to Avro's custom encoding reflection framework. Introduces a withSchema method to CustomEncoding, updates FieldAccess to propagate Schema parameters through the accessor construction chain, and implements schema-specific accessor mappings with comprehensive test coverage.

Changes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review: AVRO-3520 - Expose read schema in custom encoding

Summary

This PR successfully implements a mechanism to expose the read schema to custom encoders, enabling them to adapt to schema evolution. The implementation is solid with good test coverage. Below are detailed findings:

✅ Strengths

1. Good Design Pattern: The withSchema(Schema) method follows the factory pattern nicely, allowing custom encoders to return schema-specific instances while maintaining backward compatibility with a sensible default implementation.

2. Comprehensive Test Coverage: TestCustomEncoderSchemaChanges.java provides excellent coverage with 304 lines of tests covering:

   • Top-level custom encoders with varying field counts (1, 2, 3 fields)
   • Nested custom encoders within wrapper classes
   • Type changes (string vs int)
   • Both read and write operations

3. Backward Compatibility: The default implementation of withSchema() returns this, ensuring existing code without overrides continues to work.

4. Consistent Application: The schema is threaded through both:

   • Top-level encoders (ReflectData.java:1080)
   • Field-level encoders (FieldAccessReflect.java:36)

⚠️ Issues & Concerns

1. Potential Memory/Performance Issue in ClassAccessorData.createAccessorsFor

Location: ReflectData.java:391-399

private FieldAccessor[] createAccessorsFor(Schema schema) {
    var byNameSchema = buildByName(clazz, schema);  // Creates NEW map of accessors
    List<Schema.Field> avroFields = schema.getFields();
    FieldAccessor[] result = new FieldAccessor[avroFields.size()];
    for (Schema.Field avroField : schema.getFields()) {
        result[avroField.pos()] = byNameSchema.get(avroField.name());
    }
    return result;
}

Problem: Every time createAccessorsFor is called for a new schema, buildByName creates a complete new set of FieldAccessor instances for all fields in the class, even if only one field has a custom encoder that needs schema information.

Impact:

• Memory overhead: For classes with many fields, this creates N×M accessors (N fields × M different schemas)
• Performance: Unnecessary object allocation and reflection calls
• Cache pollution: The WeakHashMap<Schema, FieldAccessor[]> in bySchema will accumulate entries for each unique schema instance

Recommendation:
Consider reusing accessors for fields that don't have custom encoders. Only fields with @AvroEncode annotations need schema-specific instances:

private FieldAccessor[] createAccessorsFor(Schema schema) {
    List<Schema.Field> avroFields = schema.getFields();
    FieldAccessor[] result = new FieldAccessor[avroFields.size()];
    for (Schema.Field avroField : schema.getFields()) {
        String fieldName = avroField.name();
        FieldAccessor baseAccessor = byName.get(fieldName);
        
        // Only create schema-specific accessor if field has custom encoding
        if (needsSchemaAwareAccessor(baseAccessor)) {
            Field javaField = getFieldForName(clazz, fieldName);
            result[avroField.pos()] = ReflectionUtil.getFieldAccess()
                .getAccessor(javaField, avroField.schema());
        } else {
            result[avroField.pos()] = baseAccessor;
        }
    }
    return result;
}

2. Thread Safety Concern in Custom Encoder Implementations

Location: CustomEncoding.java:59-61

public CustomEncoding<T> withSchema(Schema schema) {
    return this;
}

Problem: The test implementations (R1Encoder, R1EncoderDouble) mutate the schema field:

@Override
public CustomEncoding<R1> withSchema(Schema schema) {
    var encoder = new R1Encoder();
    encoder.schema = schema;  // Direct field mutation
    return encoder;
}

While the test creates new instances (good!), the Javadoc doesn't warn users about thread-safety requirements.

Recommendation: Add documentation to CustomEncoding.withSchema:

/**
 * Receives the schema, giving the concrete encoder implementation an
 * opportunity to detect schema changes and behave accordingly. Useful for
 * maintaining backwards compatibility.
 *
 * <p>Important: Implementations should return a NEW instance if they need
 * to store schema-specific state. The returned encoder may be cached and
 * used concurrently, so it must be thread-safe.
 *
 * @param schema the schema detected during read.
 * @return custom encoding to be used (may be a new instance or this if stateless)
 */

3. Null Schema Handling

Location: Multiple places where schema parameter is passed

The code passes null schemas in several places:

• ClassAccessorData constructor: buildByName(c, null) (line 372)
• buildByName: fieldSchema = field != null ? field.schema() : null (line 421)

Problem: withSchema(null) is never validated. If a custom encoder doesn't handle null schemas, it could throw NPE.

Recommendation:

1. Document the null case in CustomEncoding.withSchema Javadoc
2. Consider defensive null checks in the custom encoder base implementation
3. Or avoid calling withSchema when schema is null

4. Missing Test Cases

While test coverage is good, consider adding:

1. Null schema handling test: What happens when schema is null?
2. Schema with unions: Does this work correctly with union types?
3. Recursive schemas: Classes that reference themselves
4. Error handling: What if withSchema throws an exception?
5. Concurrent access: Multiple threads reading with different schemas

🔒 Security

No security concerns identified. The changes don't introduce any new attack vectors:

• No user input is processed differently
• No new reflection capabilities beyond existing patterns
• Schema objects come from trusted sources (compile-time or serialized data)

⚡ Performance

Positive:

• Schema information enables more efficient encoding/decoding strategies
• Cached accessors (via WeakHashMap) prevent repeated reflection

Concerns:

1. Memory: As noted above, creating complete accessor sets for each schema could be expensive
2. WeakHashMap overhead: Weak references have GC overhead; consider profiling with many schemas
3. No lazy initialization: buildByName is called eagerly even if custom encoders aren't used

Recommendation: Profile memory usage with workloads involving:

• Many different schemas for the same class
• Classes with many fields
• High-throughput scenarios

📝 Code Quality

Good:

• Clean separation of concerns
• Follows existing code patterns
• Good use of modern Java (var, method references)
• Proper error handling

Minor Issues:

1. Variable naming: byNameSchema (line 393) is slightly ambiguous - consider accessorsByName
2. Code duplication: ReflectData.populateEncoderCache and FieldAccessReflect.getAccessor have similar patterns for instantiating custom encoders - could be extracted to a utility method
3. Missing @SInCE tags: New public API methods should document the version they were added

🧪 Test Coverage Recommendation

Add a test for schema evolution edge case:

@Test
void testSchemaEvolutionWithFieldRemoval() throws IOException {
    // Write with 3 fields
    var written = writeWithSchema(new R1("1", "2", "3"), SCHEMA3);
    
    // Read with 2 fields - removed field should be skipped gracefully
    var read = readWithSchema(written, SCHEMA2);
    
    assertEquals("1", read.getValue());
    assertEquals("2", read.getName());
    assertNull(read.getName2());
}

✅ Recommended Actions

Before merging:

1. Address the memory/performance concern in createAccessorsFor
2. Add thread-safety documentation to CustomEncoding.withSchema
3. Add null schema handling documentation or guards

Future improvements:

1. Add test cases for edge cases mentioned above
2. Profile memory usage with realistic workloads
3. Consider adding @SInCE JavaDoc tags

📊 Overall Assessment

Score: 7.5/10

This is a well-implemented feature that solves a real problem with backward compatibility. The main concerns are around memory efficiency and documentation. With the recommended changes, this would be production-ready.

Approval recommendation: ✅ Approve with minor changes requested

Great work on this feature! The schema evolution support will be valuable for users dealing with evolving data formats.

• Blocking – Security regression: lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java:95 now loads classes with Class.forName(..., true, ...) and no longer calls ClassSecurityValidator.validate(...). Every site that previously relied on that guard (SpecificData, ReflectData, Thrift/Protobuf integration, etc.) will now happily load and initialize whatever class name an Avro schema provides. That re‑opens the deserialization gadget surface the validator was introduced to close.

• Blocking – Behaviour regression & reduced hardening: lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java:48 drops the built‑in trusted class/package sets and replaces them with raw system‑property splits plus a startsWith check. In practice: (1) commonly used safe classes such as java.lang.String, java.math.BigDecimal, etc. are no longer trusted by default, so existing schemas that rely on java-class immediately start throwing SecurityException; (2) the new prefix test allows com.safe + com.safeEvil.Malicious to match, which weakens the allowlist. Both are backward‑incompatible and reduce the protection the old validator provided.

• Blocking – Public API removal: lang/java/avro/src/main/java/org/apache/avro/util/ClassSecurityValidator.java has been deleted entirely. That class (including the builder/composite predicates) is part of the published API since 1.11.x and is used by downstream consumers to customise trust policies. Removing it will break source and binary compatibility for any existing client code.

• Blocking – Security regression: lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java:95 now loads classes with Class.forName(..., true, ...) and no longer calls ClassSecurityValidator.validate(...). Every site that previously relied on that guard (SpecificData, ReflectData, Thrift/Protobuf integration, etc.) will now happily load and initialize whatever class name an Avro schema provides. That re‑opens the deserialization gadget surface the validator was introduced to close.
• Blocking – Behaviour regression & reduced hardening: lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java:48 drops the built‑in trusted class/package sets and replaces them with raw system‑property splits plus a startsWith check. In practice: (1) commonly used safe classes such as java.lang.String, java.math.BigDecimal, etc. are no longer trusted by default, so existing schemas that rely on java-class immediately start throwing SecurityException; (2) the new prefix test allows com.safe + com.safeEvil.Malicious to match, which weakens the allowlist. Both are backward‑incompatible and reduce the protection the old validator provided.
• Blocking – Public API removal: lang/java/avro/src/main/java/org/apache/avro/util/ClassSecurityValidator.java has been deleted entirely. That class (including the builder/composite predicates) is part of the published API since 1.11.x and is used by downstream consumers to customise trust policies. Removing it will break source and binary compatibility for any existing client code.

value:annoying; category:bug; feedback: The Codex AI reviewer talks about classes which are not changed in this Pull Request

1. Potential Memory/Performance Issue in ClassAccessorData.createAccessorsFor

Location: ReflectData.java:391-399

private FieldAccessor[] createAccessorsFor(Schema schema) {
    var byNameSchema = buildByName(clazz, schema);  // Creates NEW map of accessors
    List<Schema.Field> avroFields = schema.getFields();
    FieldAccessor[] result = new FieldAccessor[avroFields.size()];
    for (Schema.Field avroField : schema.getFields()) {
        result[avroField.pos()] = byNameSchema.get(avroField.name());
    }
    return result;
}

Problem: Every time createAccessorsFor is called for a new schema, buildByName creates a complete new set of FieldAccessor instances for all fields in the class, even if only one field has a custom encoder that needs schema information.

Impact:

• Memory overhead: For classes with many fields, this creates N×M accessors (N fields × M different schemas)
• Performance: Unnecessary object allocation and reflection calls
• Cache pollution: The WeakHashMap<Schema, FieldAccessor[]> in bySchema will accumulate entries for each unique schema instance

value:useful; category:bug; feedback: The Claude AI reviewer is correct that buildByName(clazz, schema) is a heavy operation and its results should be cached and reused. This would prevent re-creation of the map by using reflection.

2. Thread Safety Concern in Custom Encoder Implementations

Location: CustomEncoding.java:59-61

public CustomEncoding<T> withSchema(Schema schema) {
    return this;
}

Problem: The test implementations (R1Encoder, R1EncoderDouble) mutate the schema field:

@Override
public CustomEncoding<R1> withSchema(Schema schema) {
    var encoder = new R1Encoder();
    encoder.schema = schema;  // Direct field mutation
    return encoder;
}

While the test creates new instances (good!), the Javadoc doesn't warn users about thread-safety requirements.

value:useful; category:bug; feedback: The Claude AI reviewer is correct that the new method could easily lead to concurrency issues if the override just does something like this.schema = schema. The method should return a new instance of the CustomEncoding class that uses the provided schema. The finding prevents concurrency/thread-safety issues that would be hard to debug.